Allow setting external 32khz quartz for slow rtc clock #2032

Szybet · 2024-08-29T13:11:15Z

Hello

This is just a working draft for setting the external 32khz quartz for slow rtc clock. I would like to discuss some potential fixes for issues I encountered. The final goal is to get this merged some day, some how.

So to the issues:

For this to work it is needed to enable CONFIG_RTC_CLK_SRC_EXT_CRYS for the bootloader and rebuild it, reflash it. Otherwise it's simply not turned on and the program tries to calibrate the quartz, so it freezes at that point. The possible fixes are: 2 set of bootloader binaries? A simple note that you need a custom bootloader binary?
post_init at esp-hal/src/soc/esp32c6/mod.rs turns the RTC on, I don't know why it is the case, but this requires choosing the rtc clock as the default variable in the code, as there is no config system yet. The obvious solution is to not do that? I don't know the reason behind it, but it messes things up. Now, if the user wants the rtc variable, it will be initialized / calibrated 2 times for no reason
To create the RTC now you need to specify the clock: Rtc::new(LPWR::steal(), RtcSlowClock::default()); this breaks all current examples and programs. In my experience, this will not be an issue 😃

bugadani · 2024-08-30T15:36:12Z

We might be able to place this into esp_hal::init once #1970 and its follow-up are merged.

Szybet · 2024-09-02T07:49:38Z

I'm gonna adjust this draft once the other pull request is merged then

Szybet · 2024-09-03T06:28:17Z

@bugadani I have questions on how to exactly do it

esp-hal/esp-hal/src/lib.rs

Lines 726 to 742 in 08d406e

    
           /// System configuration. 
        
           #[non_exhaustive] 
        
           #[derive(Default)] 
        
           pub struct Config { 
        
               /// The CPU clock configuration. 
        
               pub cpu_clock: CpuClock, 
        
           } 
        
           /// Initialize the system. 
        
           /// 
        
           /// This function sets up the CPU clock and returns the peripherals and clocks. 
        
           pub fn init(config: Config) -> (Peripherals, Clocks<'static>) { 
        
               let peripherals = Peripherals::take(); 
        
               let clocks = ClockControl::new(config.cpu_clock).freeze(); 
        
               (peripherals, clocks) 
        
           }

Here, I should probably add the enum RtcSlowClock to Config and then configure it in init the question is, should init always return now the Rtc class, even when the user never asked for it (as he probably wanted the default). This will introduce another "Api" change and all examples would need to be adjusted. I don't know, a possible fix for it is a struct that returns those variables instead of returning them manually.

bugadani · 2024-09-03T07:08:08Z

I'm not very familiar with the rtc API, but I think you should add RtcSlowClock to Config, call rtc::configure_clock here and remove the clock source parameter from Rtc::new. There's no need to return anything more than what we currently do.

Szybet · 2024-09-03T07:57:12Z

Oh, this will not work at all because post_init still configures the RTC clock with the default RtcSlowClock, there is no way to pass to it the config

bugadani · 2024-09-03T08:13:39Z

@MabezDev can we get rid of post_init in favour of esp_hal::init?

Szybet · 2024-09-03T08:21:47Z

Why does it exist in the first place?

Dominaezzz · 2024-09-03T09:31:16Z

To run code before main but after initialisation. Useful for stuff like psram and until now, watchdog.

Rather than get rid of the post_init , maybe just move the watchdog disabling code to the init function and have that be configurable.
That would help fix #1698 as well.
(Arguably out of scope for this PR but it should solve your problem I think)

…ove all changes made to this branch

examples/src/bin/32khz_xtal.rs

Szybet · 2024-09-04T09:40:40Z

Okay, I did it for the esp32c6 for now. Does it look fine? If yes, I can do it for the rest of the cpu's.

Also I tested it on esp32c6-devkit-m-1.0 with a soldered in xtal 32khz, tested if quartz turns on with an oscilloscope, it did. It required a special bootloader as I said, otherwise it won't output hello world.

Co-authored-by: Dániel Buga <[email protected]>

Szybet · 2024-09-06T05:53:35Z

Silence means it's fine, right 😃 ?

bugadani · 2024-09-06T06:53:49Z

I think so. Please rebase :)

bugadani · 2024-09-06T06:57:28Z

examples/src/bin/32khz_xtal.rs

@@ -0,0 +1,50 @@
+//! This turns on the 32 khz xtal and then just prints hello world to UART. Requires a special bootloader


I don't see the value in this test. For one, what's "a special bootloader" that is required for it? Next, what does this example show, that changing the clock source doesn't break the whole system?

The bootloader needs to be build with CONFIG_RTC_CLK_SRC_EXT_CRYS=y

Well yes, also allows to measure the sine wave with an osciloscope

Can we select the clock source here, instead of relying on the bootloader?

Not without losing some optimization https://github.com/espressif/esp-idf/blob/59e18382702b6986be3d3f55e9ac7763c1397cf7/components/bootloader_support/src/bootloader_clock_init.c#L82-L90

And maybe something more is behind it, I don't know

if (!rtc_clk_32k_enabled()) {

Unless that takes long to decide, this is safe to do. If the bootloader enabled the clock source, and the user selects in in esp-hap, we can skip initialization. Otherwise, in the future we might allow other bootloaders which may or may not set up the clock source, so I'd say let's enable the clock ourselves, if it's not done already.

Please note it's inside the macro statement #if CONFIG_ESP_SYSTEM_RTC_EXT_XTAL

Well maybe it is safe but we lose some optimization, I don't think we should lose any performance because at default we don't control the bootloader fully. In the future we probably will by using a rust based bootloader so it will work the same just like now, but without the hassle of recompiling it

I don't think we should lose any performance because at default we don't control the bootloader fully

Let's prefer the option that works by default. If the user has problems with the startup time, they can use a custom bootloader.

Making it work only in the application would require changing the code for it to wait to start up, not only is it not efficient but also I don't know how to do it, I looked at a lot of esp-idf code and haven't really found anything that waits for it, I ques booting the bootloader is enough time for it to start after all. Using a fixed time is also bad, would require cpu dependent testing to optimize it.

This is simply not the way to do it...

Fine. At least please print a warning or panic if the bootloader forgot to enable the clock source.

Ok, I will try to find a way to detect it (probably just failing the calibration step)

Szybet · 2024-09-09T12:40:47Z

Does this look fine? It's able to return to the default clock and everything seems to work fine, but I can't guarantee it

Also how do I enable logs from esp-hal, the log feature is already enabled when running examples

bjoernQ · 2024-09-10T06:55:03Z

To enable logs via esp-println your need to initialize the logger - e.g. esp_println::logger::init_logger_from_env(); early in your code. And you need to specify the log-level via the env variable ESP_LOGLEVEL - e.g. debug. See https://crates.io/crates/esp-println

Szybet · 2024-09-20T09:31:07Z

To enable logs via esp-println your need to initialize the logger - e.g. esp_println::logger::init_logger_from_env(); early in your code. And you need to specify the log-level via the env variable ESP_LOGLEVEL - e.g. debug. See https://crates.io/crates/esp-println

Shouldn't there be an option / feature for esp-hal to enable it by itself? The logs I want are being executed before user code. Basically show esp-hal logs if the user wants it, because otherwise it's not possible to enable them directly from the user code and I have no idea how early can I put the log init for it to work

Well anyway

Does the esp32c6 code for setting the 32 khz oscillator look fine? If yes, can I implement it for other cpu's. I don't want to rework it later for all of them again

MabezDev · 2024-09-20T15:54:39Z

Shouldn't there be an option / feature for esp-hal to enable it by itself? The logs I want are being executed before user code. Basically show esp-hal logs if the user wants it, because otherwise it's not possible to enable them directly from the user code and I have no idea how early can I put the log init for it to work

Enable it before calling esp_hal::init() and you'll get the logs. Before main almost nothing else happens, at least nothing interesting.

MabezDev · 2024-10-21T13:09:01Z

I think this PR should contain the code to initialize the external quartz before we can merge it, depending on a custom bootloader to do it isn't acceptable IMO. Is this something you're willing to work on @Szybet or shall we close this?

Szybet · 2024-10-21T14:28:23Z

I definitely want to do something with it, but working on a custom rust bootloader? that's out of my league.
We maybe could just do a delay, but it would need to be huge to make it cpu independent. Also that's just a bad solution.

Maybe just accept the custom bootloader? both probe-rs and espflash support a custom bootloader binary argument

allow setting external 32khz quartz

eb3af04

Szybet added 5 commits September 4, 2024 09:22

Merge remote-tracking branch 'notfork/main' into 32khz-quartz and rem…

836ee48

…ove all changes made to this branch

clean out the rest

ef6d813

clean out the rest even more

b497622

another implementation of choosing the 32khz quartz

3d5b8ff

rename variable

55c2d08

bugadani reviewed Sep 4, 2024

View reviewed changes

examples/src/bin/32khz_xtal.rs Outdated Show resolved Hide resolved

Update examples/src/bin/32khz_xtal.rs

eb79d6a

Co-authored-by: Dániel Buga <[email protected]>

bugadani reviewed Sep 6, 2024

View reviewed changes

fallback to default rtc clock

e1abea4

Szybet mentioned this pull request Sep 9, 2024

Add dynamic output gpio example to demonstrate how to use AnyPin and Flex #1903

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow setting external 32khz quartz for slow rtc clock #2032

Allow setting external 32khz quartz for slow rtc clock #2032

Szybet commented Aug 29, 2024

bugadani commented Aug 30, 2024

Szybet commented Sep 2, 2024

Szybet commented Sep 3, 2024

bugadani commented Sep 3, 2024

Szybet commented Sep 3, 2024

bugadani commented Sep 3, 2024

Szybet commented Sep 3, 2024 •

edited

Loading

Dominaezzz commented Sep 3, 2024

Szybet commented Sep 4, 2024

Szybet commented Sep 6, 2024

bugadani commented Sep 6, 2024

bugadani Sep 6, 2024

Szybet Sep 6, 2024

bugadani Sep 6, 2024

Szybet Sep 6, 2024

bugadani Sep 6, 2024

Szybet Sep 6, 2024

bugadani Sep 6, 2024

Szybet Sep 6, 2024 •

edited

Loading

bugadani Sep 6, 2024

Szybet Sep 6, 2024

Szybet commented Sep 9, 2024

bjoernQ commented Sep 10, 2024

Szybet commented Sep 20, 2024

MabezDev commented Sep 20, 2024

MabezDev commented Oct 21, 2024

Szybet commented Oct 21, 2024

		@@ -0,0 +1,50 @@
		//! This turns on the 32 khz xtal and then just prints hello world to UART. Requires a special bootloader

Allow setting external 32khz quartz for slow rtc clock #2032

Are you sure you want to change the base?

Allow setting external 32khz quartz for slow rtc clock #2032

Conversation

Szybet commented Aug 29, 2024

bugadani commented Aug 30, 2024

Szybet commented Sep 2, 2024

Szybet commented Sep 3, 2024

bugadani commented Sep 3, 2024

Szybet commented Sep 3, 2024

bugadani commented Sep 3, 2024

Szybet commented Sep 3, 2024 • edited Loading

Dominaezzz commented Sep 3, 2024

Szybet commented Sep 4, 2024

Szybet commented Sep 6, 2024

bugadani commented Sep 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Szybet Sep 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Szybet commented Sep 9, 2024

bjoernQ commented Sep 10, 2024

Szybet commented Sep 20, 2024

MabezDev commented Sep 20, 2024

MabezDev commented Oct 21, 2024

Szybet commented Oct 21, 2024

Szybet commented Sep 3, 2024 •

edited

Loading

Szybet Sep 6, 2024 •

edited

Loading